Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature for basis u compilation with SSE 4.1 on supported platforms #14

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

julhe
Copy link
Contributor

@julhe julhe commented May 20, 2023

Should improve encoder times 15-30% acording to the original repo on supported platforms (virtually any x86 CPU made after 2007).

Whether basisu is built with SSE4.1 support is simply dictated by the build target platform, so no need to create an extra crate feature for this in my opinion. 🙂

@aclysma
Copy link
Owner

aclysma commented May 22, 2023

Steam hardware survey has support at >99% so I think this is fine. If the CI fails again I'll grab and test this on mac myself. Thanks!

@aclysma
Copy link
Owner

aclysma commented May 22, 2023

(I think bevy is using this repo so I asked in their discord for anyone with objections to speak up.)

@fintelia
Copy link
Contributor

It seems preferable to check whether sse4.1 is listed in CARGO_CFG_TARGET_FEATURE. That will indicate whether the user requested rustc to produce binaries containing SSE 4.1 instructions, rather than simply inferring it by what ISA is used on the build machine.

@@ -15,10 +15,12 @@ fn build_with_common_settings() -> cc::Build {
}

fn main() {
let sse_support = cfg!(target_feature = "sse4.1");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that cfg! will detect the features used to build the build script itself, which are not necessarily the same as the target building for (for example when --target is used Cargo will not pass the rustflags to the build script, but it will pass them to the crate you're building for). It's suggested instead to read the CARGO_CFG_TARGET_FEATURE environment variable, which will properly report the target features enabled for the target you're building for.

Something like:

let sse4.1 = std::env::var("CARGO_CFG_TARGET_FEATURE")
	.expect("Cargo didn't set the CARGO_CFG_TARGET_FEATURE env var")
	.split(',')
	.any(|feature| feature == "sse4.1");

@aclysma
Copy link
Owner

aclysma commented May 24, 2023

Someone please correct me if I get this wrong but the tier 1 targets in rust are x86_64, i686, and aarch64. You can check features with something like rustc --print=cfg -C target-cpu=x86-64. On windows it looks like this

debug_assertions
panic="unwind"
target_arch="x86_64"
target_endian="little"
target_env="msvc"
target_family="windows"
target_feature="fxsr"
target_feature="sse"
target_feature="sse2"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="ptr"
target_os="windows"
target_pointer_width="64"
target_vendor="pc"
windows

None of these tier 1 targets include sse4.1. So while checking the target does seem like the intended solution, in practice I think this will mean almost no one will enable sse4.1. (I expect most people take the default target for their OS.)

This is not to say I object to going with this, but I think it's worth mentioning.

@fintelia
Copy link
Contributor

The default target-cpu for each of the tier 1 targets doesn't include sse4.1. However, you can set a different target-cpu via RUSTFLAGS. You can see the list of supported CPUs to pick from with something like:

$ rustc --target=x86_64-unknown-linux-gnu --print target-cpus

The list includes a couple of "x86-64 microarchitecture levels" in addition to named cpu models. Actually using it would look something like:

$ RUSTFLAGS='-C target-cpu=x86-64-v2' cargo run

(You can instead create a .cargo/config in the root of your repository if you don't want to write it out every time)

@aclysma
Copy link
Owner

aclysma commented May 29, 2023

Based on the input so far, going to hold off on taking this PR until it is opt-in, preferably by checking CARGO_CFG_TARGET_FEATURE

@julhe
Copy link
Contributor Author

julhe commented May 30, 2023

Thanks for all your input!

I think letting the user pass compiler flags is too arcane, so I exposed it as a regular feature hint_sse4_1 + an additional check if we are building for x86 or x86_64. So, if the hint_sse4_1 flag is set and where building for non-x86, no SEE 4.1 is used. That should prevent headaches when building for non x86 platforms.

@julhe julhe changed the title Compile basisu with SSE 4.1 on supported platforms automatically Add feature for basis u compilation with SSE 4.1 on supported platforms May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants